feat: coerce nested dataclass fields in Relation.load by tonyandrewmeyer · Pull Request #2741 · canonical/operator

tonyandrewmeyer · 2026-09-10T04:46:53Z

Relation.load passes the decoded relation data straight to the data class's __init__, so a field whose own type is a dataclass or an enum arrives as a plain dict or str, and the attribute access the charm does next fails. This PR changes that to coerce each field against its type hint before construction, recursively, for plain dataclasses only - Pydantic models and pydantic dataclasses keep doing their own coercion and validation.

list, set and frozenset coerce their elements; a fixed-length tuple coerces each position against its own type and stays a tuple; dict/Mapping coerce their values.
An Optional/Union field is coerced against its single non-None member. A union of more than one concrete type is passed through as-is, since there's no way to tell which member to coerce against.
Where the type hints can't be resolved at all, construction falls back to the uncoerced path rather than failing.

Split out of #2557, which is where this came from and which needs it: ops_tracing's de-vendored databag models have nested dataclasses one level deep, and its suite doesn't pass without this. It seems like it's valuable outside of that (if we end up going with using the charmlibs libraries, for example), and reviewing it separately makes it easier to review #2557 focussing specifically on what it's about.

Split out of canonical#2557, where this shipped alongside the ops_tracing de-pydantic work. Relation.load now recursively constructs nested dataclasses and coerces Enum field values when the target is a non-pydantic dataclass, so callers get typed nested objects instead of raw decoded dicts. This carries over the original implementation from canonical#2557 unmodified; the review at REVIEW-2557.md found several holes in it, fixed in the commits that follow. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J1dP9XqjiuoYz4oHjWYAQ6

….load _coerce_field returned early for anything whose typing.get_origin wasn't list/tuple/set/frozenset, which includes Optional[X] (a Union) and dict[str, X] - both common shapes for nested-model fields. A charm reading data.inner.a on an Optional[Nested] field got an AttributeError instead, since inner stayed a plain dict. Coerce a Union against its single non-None member (a Union of more than one concrete type has no way to pick a target, so it still passes through as-is), and a dict/Mapping's values against the value type. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J1dP9XqjiuoYz4oHjWYAQ6

…ionally origin in (list, tuple) shared one branch, so tuple[X, ...] fields came back as a list, and every element of a fixed-length tuple[X, Y] was coerced against args[0], leaving Y untouched. Split the branch: a tuple stays a tuple, a variable-length tuple[X, ...] (args[-1] is Ellipsis) coerces every element against X, and a fixed-length tuple coerces each position against its own type. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J1dP9XqjiuoYz4oHjWYAQ6

The guard used __is_pydantic_dataclass__, which only exists from pydantic 2.11. Measured across installed versions, it's absent on 2.0.3, 2.4.2, 2.6.4 and 2.10.6. ops declares no pydantic dependency - the charm's own pin decides - and ops's test extra permits 2.10.x, so a charm pinned there got ops's pre-coercion applied ahead of pydantic's own validators, and any extra kwargs pydantic would have accepted silently dropped. '__pydantic_validator__' in cls.__dict__ is what pydantic.dataclasses.is_pydantic_dataclass itself checks for, and was present on every version tested from 2.0.3 up. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J1dP9XqjiuoYz4oHjWYAQ6

… be resolved get_type_hints(cls) resolves every field's annotation eagerly, so a TYPE_CHECKING-only import with no runtime name raised NameError even for relation data that never touched the affected field - a regression against main's cls(**data) path, which didn't need the hints at all. ops's own ruff config disables TC001/2/3, so charms following ops's own conventions are the ones most likely to hit this. Fall back to the un-coerced cls(**data) when hints can't be resolved. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J1dP9XqjiuoYz4oHjWYAQ6

…ional args The `not args` guard meant any positional argument silently turned off coercion entirely: relation.load(MyData, event.app) coerced, and relation.load(MyData, event.app, something) did not, with nothing telling the caller so. args are matched to cls's leading dataclass fields by position, so there is nothing to coerce them against - but the remaining fields, still supplied from the relation data, can and should keep being coerced. _build_dataclass now takes the positional args through and only skips coercion for the fields they fill. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J1dP9XqjiuoYz4oHjWYAQ6

The docstring still said the data "is passed to the data class's __init__ method as keyword arguments", which after recursive coercion holds only for pydantic targets and flat dataclasses. State which field types are coerced, which pass through unchanged, and what happens when positional arguments or unresolvable type hints are involved. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J1dP9XqjiuoYz4oHjWYAQ6

Three problems with _coerce_field, all from review of canonical#2741: An Optional[X] field whose databag value is null was coerced against X, which cannot accept None: a nested dataclass raised TypeError from the membership test, a list raised on iteration, and an enum raised ValueError. None now short-circuits, so `field: Nested | None` with `null` in the databag gives None as it did before coercion existed. A frozenset[X] field was built with a set comprehension, so it always produced a set. The set and frozenset branches are now separate and each produces its own type. A str, bytes or mapping value for a sequence field was iterated element-wise, quietly producing a list of characters or of the mapping's keys rather than failing. Those three are now rejected with a TypeError naming the expected type, as is a non-mapping value for a dict field, which previously surfaced as AttributeError: 'list' object has no attribute 'items'. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`_build_dataclass` chooses which fields to fill with `field.name not in data`, which is False for every field of a string or a list, so a remote app writing `"oops"` where a nested dataclass belongs got a confidently default-constructed object corresponding to nothing in the databag - and `"subscribe"`, which happens to contain `sub`, got "string indices must be integers" from inside ops instead. A charm can't stop a remote app writing nonsense, so this is the path that matters. The nested-dataclass branch now rejects a non-mapping the way the sequence and mapping branches already do, naming the class and what arrived. A value that is already an instance of the field's class is passed through: `Relation.load`'s keyword arguments go through the same coercion as the databag, and they are documented as passed through to the data class, so `load(Data, app, nested=Nested(sub=5))` must not try to build a `Nested` out of a `Nested`. `_juju_fields` also keys its pydantic check off `__pydantic_validator__` rather than `__is_pydantic_dataclass__`, which is what `Relation.load` already does and what `pydantic.dataclasses.is_pydantic_dataclass` itself checks. `__is_pydantic_dataclass__` only exists from pydantic 2.11, so on 2.10 an aliased field came back under its field name: measured against pydantic 2.10.6, `_juju_fields` gave `{'secret-id': 'secret_id'}` before this and `{'secret-id': 'secret-id'}` after, matching 2.13.4 either way. ops's own test extra is `pydantic~=2.10`, so both are in scope. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

james-garner-canonical · 2026-09-17T22:55:25Z

-            # Using pydantic.dataclasses.is_pydantic_dataclass() would be
-            # best here, but we don't want to import pydantic in ops, so
-            # we look more explicitly.
-            if getattr(cls, '__is_pydantic_dataclass__', False):
+            # Using pydantic.dataclasses.is_pydantic_dataclass() would be best
+            # here, but we don't want to import pydantic in ops, so we check
+            # for the attribute that function itself checks for. Note that
+            # '__is_pydantic_dataclass__' only exists from pydantic 2.11, so
+            # relying on that one misses every earlier 2.x pydantic dataclass,
+            # which reads the aliases back under their field names instead.
+            if '__pydantic_validator__' in cls.__dict__:


Worth landing this fix in a separate PR?

I can if you prefer. It seems small enough (and I suspect non-critical but I didn't actually check what versions charms are using) to bundle in here. But it is a separate fix so I can make a little PR only for that.

Let's do the split.

james-garner-canonical

Some feedback on the details. Still digesting the big picture behaviour change.

james-garner-canonical · 2026-09-17T23:02:33Z

+        - ``Literal`` fields, and any other constructed generic not listed
+          above, are passed through unchanged.


I wonder if we can do better here. For example, it seems something like Literal[1] should at least be treated like int. Since Literal only accepts pretty basic types, this is very tractable. On the other hand, if the databag was properly JSON-serialized, then you'd have the correct type (int) here anyway.

Literal[1] would get treated as int here, but that wouldn't actually do anything: _coerce_field only builds things (nested dataclasses, enums, collections) and passes any other scalar type straight through, so an int annotation is already a no-op.

Or do you mean if the databag had '1' but the type was int it should end up as 1? I'm pretty hesitant about that, it seems like it could introduce all sorts of problems, and isn't very Pythonic.

Literal[1] would get treated as int here, but that wouldn't actually do anything: _coerce_field only builds things (nested dataclasses, enums, collections) and passes any other scalar type straight through, so an int annotation is already a no-op.

True, that makes sense. So maybe that should just be the comment about Literal -- not sure though.

Or do you mean if the databag had '1' but the type was int it should end up as 1? I'm pretty hesitant about that, it seems like it could introduce all sorts of problems, and isn't very Pythonic.

I think Pydantic models do this if the model hasn't configured strict mode FWIW. I don't think we should follow suit.

james-garner-canonical · 2026-09-17T23:11:10Z

+        - ``list``, ``set``, and ``frozenset`` fields coerce each element
+          against the type argument.
+        - A variable-length ``tuple[X, ...]`` coerces every element against
+          ``X``; a fixed-length ``tuple[X, Y, ...]`` coerces each position
+          against its own type, and stays a ``tuple``.
+        - A ``dict``/``Mapping`` field coerces its values against the value
+          type.


Document once as collections, listing the supported collection types. Explain fixed-length tuples as a separate follow-up bullet point, explaining what happens if there are more values than annotated positions.

james-garner-canonical · 2026-09-17T23:16:26Z

+        - An ``Optional``/``Union`` field is coerced against its single
+          non-``None`` member; a ``Union`` of more than one concrete type is
+          passed through as-is, since there is no way to tell which member to
+          coerce against.


I wonder if we can do better for unions. For example, a list[SomeDataClass] | dict[int, SomeDataClass] can be resolved since we either get a value like [{...}] or {1: {...}}.

I'm also thinking about cases like SomeEnum | int or SomeDataClass | str -- or SomeDataClass | SomeEnum and the value is a string and SomeDataClass(val) and SomeEnum(val) would both resolve cleanly. Would trying left to right be problematic? How does Pydantic handle this -- I'd guess left to right, but I haven't checked.

I'd also be OK with drawing a clean boundary somewhere and documenting the current limitations and possible extension points, but I'm not sure the current implementation is the right place to draw that line.

james-garner-canonical · 2026-09-17T23:18:53Z

+        If the class's type hints can't be resolved at all - for example, a
+        ``TYPE_CHECKING``-only import with no runtime name - the values are
+        passed through uncoerced instead of raising.


I guess stringified annotations may also end up here? Pydantic itself faces similar troubles https://pydantic.dev/docs/validation/latest/internals/resolving_annotations/#the-challenges-of-runtime-evaluation

james-garner-canonical · 2026-09-17T23:25:07Z

+        Any additional positional or keyword arguments will be passed through
+        to the data class ``__init__``. For a non-pydantic dataclass target,
+        positional arguments are matched to the class's leading fields by
+        position; those fields are passed through as given rather than
+        coerced, since there is no field name to coerce them against, but any
+        remaining fields supplied from the relation data are still coerced as
+        above.


I think user-supplied **kwargs to this function shouldn't be coerced at all, both for symmetry with user-supplied *args and just on general principle -- the user can (and should) just supply fully-resolved arguments.

This is a bit problematic with the design here of having the user-supplied arguments be overridden by those from the databag on collision, but definitely not unresolveable -- build data starting from a plain dict, combine as {**kwargs, **data} for the Pydantic case, and pass separately as data and {k for k in kwargs if k not in data} for dataclasses.

james-garner-canonical · 2026-09-17T23:26:07Z

+    return value
+
+
+def _build_dataclass(cls: Any, data: Mapping[str, Any], *args: Any) -> Any:


There's no need to accept args as variadic -- it just adds a * at the call-site.

james-garner-canonical · 2026-09-17T23:34:49Z

+            and args
+            and isinstance(value, (str, bytes, Mapping))
+        ):
+            given = cast('Any', value)


value seems to already be annotated as Any at the function scope. The isinstance narrows it to str | bytes | Mapping. Mapping[Unknown] perhaps? (Thanks, Pydantic!) -- is that why we need a cast here to be able to call type? Rather than a cast, I think we'd be better off here with something like given_type = type(value) # pyright: ignore[...].

james-garner-canonical · 2026-09-17T23:43:22Z

    raise ValueError('Unable to find class fields')


+def _coerce_field(tp: Any, value: Any) -> Any:


Would _coerce_field(tp: type[T], value: Any) -> T: work?

Looking further, this seems to be complicated by accepting annotation types like Optional and so on as well.

What do you think about breaking this down into a pair of helpers: one that accepts complex type annotations, resolved the inner types, and so on, and a separate function that takes only fully-resolved types?

james-garner-canonical · 2026-09-17T23:45:20Z

+    origin = typing.get_origin(tp)
+    if origin is not None:


At a glance its' really tough to tell which branches are terminal and where we fall through. Factoring into helpers might help. For example if origin is None here we could return _coerce_resolved(tp, value).

claude added 7 commits September 9, 2026 15:07

tonyandrewmeyer commented Sep 10, 2026

View reviewed changes

Comment thread ops/model.py Outdated

Apply suggestion from @tonyandrewmeyer

d81e1cd

tonyandrewmeyer marked this pull request as ready for review September 10, 2026 04:53

tonyandrewmeyer requested review from dwilding and james-garner-canonical September 10, 2026 04:53

dwilding reviewed Sep 15, 2026

View reviewed changes

Comment thread ops/charm.py

Comment thread ops/charm.py

Comment thread ops/charm.py

tonyandrewmeyer and others added 2 commits September 16, 2026 09:32

james-garner-canonical reviewed Sep 17, 2026

View reviewed changes

		- ``Literal`` fields, and any other constructed generic not listed
		above, are passed through unchanged.

		return value


		def _build_dataclass(cls: Any, data: Mapping[str, Any], *args: Any) -> Any:

		raise ValueError('Unable to find class fields')


		def _coerce_field(tp: Any, value: Any) -> Any:

Conversation

tonyandrewmeyer commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

james-garner-canonical left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tonyandrewmeyer commented Sep 10, 2026 •

edited

Loading